fix(studio): support drag on FlatSlider, not just click-to-set#2186
Conversation
miga-heygen
left a comment
miga-heygen
left a comment
There was a problem hiding this comment.
Reviewed as part of the 15-PR edit-panel redesign stack. Full stack review posted on #2120. No blockers on this PR. — Miga
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Final current-head pass: ad36891. Reviewed the stack boundaries and current diff against the flat-inspector contracts; required checks have no failures/pending checks and no unresolved review threads remain. No new drift found; residual notes are non-blocking.
vanceingalls
force-pushed
the
studio-flat-12-slider-hit-area
branch
from
a88b530 to
fd1fb17
Compare
vanceingalls
force-pushed
the
studio-flat-13-slider-drag
branch
from
ad36891 to
102c503
Compare
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Superseding earlier approval: the latest max-review pass found confirmed correctness issues in this flat-inspector stack (identity/selector-index state, hide-all write races, timing inference, slider pointer/keyboard/reset semantics, and duplicate React keys). Hold merge and require fixes plus re-review on the current stack head.
vanceingalls
force-pushed
the
studio-flat-13-slider-drag
branch
from
102c503 to
8b2b91c
Compare
vanceingalls
force-pushed
the
studio-flat-12-slider-hit-area
branch
from
fd1fb17 to
1c3ebfe
Compare
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 8b2b91c8635b7c5eaa46294c6827ba5901345004. The drag implementation still has an incomplete interaction lifecycle:
propertyPanelFlatPrimitives.tsx:331-342can commit on pointer-up without the disabled guard used by down/move.- The pointer-only slider has no native keyboard parity,
pointercancel/lostpointercapturecompletion, or touch-action contract, so cancellation/scrolling can skip the final commit and leave state/capture inconsistent. - The reset affordance is not disabled consistently.
Use a native range or implement the full focus/keyboard/pointer cancellation lifecycle and test disabled release, pointer cancellation, touch, and keyboard behavior.
Verdict: REQUEST CHANGES
Reasoning: The new drag path can commit while disabled and lacks required cancellation/accessibility lifecycle semantics.
— Deepwork
vanceingalls
force-pushed
the
studio-flat-13-slider-drag
branch
from
8b2b91c to
7a2ccdc
Compare
vanceingalls
force-pushed
the
studio-flat-12-slider-hit-area
branch
from
1c3ebfe to
415ac65
Compare
|
Status on this review's findings, checked against the current stack tip: Already fixed downstream, in PR #2225:
Fixed in PR #2416, commit
Not changed: |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed at 7a2ccdcc66b71a6ebbd02ae845b55bb7db8aea19.
The incremental patch is byte-equivalent to the previously reviewed head (patch-id c4b10641…). propertyPanelFlatPrimitives.tsx:257-342 still lacks complete pointercancel/lostpointercapture, touch-action, keyboard, and disabled-release handling. Reset can still mutate disabled controls. These are reachable lifecycle/accessibility failures, not rebase artifacts, and no requested regression tests landed.
Verdict: REQUEST CHANGES
Reasoning: The same incomplete pointer/keyboard/disabled contract remains at the exact current head.
— Deepwork
|
Confirming: this PR's own head is unchanged, correctly — the fixes for this review's findings were not squashed backward into it. They land in PR #2225 and PR #2416, which sit directly on top of this PR in the same Graphite stack ( This is a stacked-PR review gate question, not a missing-fix question: the stack is merged bottom-up in one sitting via Graphite ( Evaluating each intermediate head against "is this correct if it were the final merged state" doesn't match how the stack actually ships. Could you review mergeability at the stack tip (#2416, which contains every fix) rather than blocking each intermediate PR on findings that are already resolved a few commits further up the same stack? Happy to walk through the specific commit that fixes any finding you want re-verified at the tip. |
A fresh full-stack re-review checked 15 PR heads independently. Cross- checked all 9 remaining claims against the actual current tip: - #2120 (id/selector key qualification, Hide All no-op/race, variable parity), #2121 (opacity-zero fallback), #2122 (GSAP preview sibling resolution, scrub-label), #2124/#2126 (negative metadata cache), and the keyboard-access half of #2121/#2186 were all already fixed by a later commit in this same stack (65954c3, PR #2225) — the reviewed heads predate it. Verified each in the current source rather than taking the isolated-head review at face value. - #2186's "no ESC/right-click cancel during drag" was the one claim that held up: FlatSlider had keyboard arrow-key support but no way to abort an in-progress pointer drag. Escape now reverts to the pre-drag value and releases pointer capture; a right-click (contextmenu) during a drag does the same instead of committing whatever position the pointer last reached while the native context menu opens over the slider. Both go through commitDraft (not just a visual reset) since the drag's leading- edge commit in onPointerDown may already have applied an intermediate value that needs actually undoing, not just hiding. propertyPanelFlatPrimitives.tsx crossed the 600-line file-size gate after this change; extracted FlatSelectRow into its own file, matching the FlatToggle/FlatMaskInsetRows precedent from earlier in this stack. New regression tests for Escape-cancel and contextmenu-cancel. Full studio suite still at the known pre-existing 55-failure baseline, zero new regressions. Typecheck/oxlint/oxfmt clean.
|
Re-checked both claims against the current stack tip:
Both resolved as of the current tip. |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Stack-scoped re-review at 7a2ccdc.
The incremental layer is unchanged, and its prior requested changes are now resolved in the accepted mandatory tip fix, #2416 at 832d5be. I verified the cumulative tip closes the flat-inspector identity, value-parity, lifecycle, persistence, accessibility, and constrained-layout findings with focused regression coverage. This approval assumes the Graphite stack lands bottom-up through #2416; this layer should not be landed while dropping the approved tip fix.
Verdict: APPROVE
Reasoning: The accepted tip-fix strategy closes the blocker inherited or introduced at this layer, and the verified cumulative stack is coherent through #2416.
— Deepwork
Only onPointerDown was wired, so dragging the knob/track only ever committed the initial click position — nothing tracked the pointer after that. Uses the Pointer Capture API (setPointerCapture on pointerdown, onPointerMove while captured, release on pointerup) so the value follows the cursor continuously during a drag, matching how the legacy native <input type="range"> control behaves for free. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
vanceingalls
force-pushed
the
studio-flat-12-slider-hit-area
branch
from
415ac65 to
ce07dfc
Compare
A fresh full-stack re-review checked 15 PR heads independently. Cross- checked all 9 remaining claims against the actual current tip: - #2120 (id/selector key qualification, Hide All no-op/race, variable parity), #2121 (opacity-zero fallback), #2122 (GSAP preview sibling resolution, scrub-label), #2124/#2126 (negative metadata cache), and the keyboard-access half of #2121/#2186 were all already fixed by a later commit in this same stack (65954c3, PR #2225) — the reviewed heads predate it. Verified each in the current source rather than taking the isolated-head review at face value. - #2186's "no ESC/right-click cancel during drag" was the one claim that held up: FlatSlider had keyboard arrow-key support but no way to abort an in-progress pointer drag. Escape now reverts to the pre-drag value and releases pointer capture; a right-click (contextmenu) during a drag does the same instead of committing whatever position the pointer last reached while the native context menu opens over the slider. Both go through commitDraft (not just a visual reset) since the drag's leading- edge commit in onPointerDown may already have applied an intermediate value that needs actually undoing, not just hiding. propertyPanelFlatPrimitives.tsx crossed the 600-line file-size gate after this change; extracted FlatSelectRow into its own file, matching the FlatToggle/FlatMaskInsetRows precedent from earlier in this stack. New regression tests for Escape-cancel and contextmenu-cancel. Full studio suite still at the known pre-existing 55-failure baseline, zero new regressions. Typecheck/oxlint/oxfmt clean.
vanceingalls
force-pushed
the
studio-flat-13-slider-drag
branch
from
7a2ccdc to
c699778
Compare
vanceingalls
dismissed
miguel-heygen’s stale review
The base branch was changed.
A fresh full-stack re-review checked 15 PR heads independently. Cross- checked all 9 remaining claims against the actual current tip: - #2120 (id/selector key qualification, Hide All no-op/race, variable parity), #2121 (opacity-zero fallback), #2122 (GSAP preview sibling resolution, scrub-label), #2124/#2126 (negative metadata cache), and the keyboard-access half of #2121/#2186 were all already fixed by a later commit in this same stack (65954c3, PR #2225) — the reviewed heads predate it. Verified each in the current source rather than taking the isolated-head review at face value. - #2186's "no ESC/right-click cancel during drag" was the one claim that held up: FlatSlider had keyboard arrow-key support but no way to abort an in-progress pointer drag. Escape now reverts to the pre-drag value and releases pointer capture; a right-click (contextmenu) during a drag does the same instead of committing whatever position the pointer last reached while the native context menu opens over the slider. Both go through commitDraft (not just a visual reset) since the drag's leading- edge commit in onPointerDown may already have applied an intermediate value that needs actually undoing, not just hiding. propertyPanelFlatPrimitives.tsx crossed the 600-line file-size gate after this change; extracted FlatSelectRow into its own file, matching the FlatToggle/FlatMaskInsetRows precedent from earlier in this stack. New regression tests for Escape-cancel and contextmenu-cancel. Full studio suite still at the known pre-existing 55-failure baseline, zero new regressions. Typecheck/oxlint/oxfmt clean.
3 participants
What
Thirteenth PR in the flat inspector stack (see #2120 for the foundation and full stack list). Adds real drag support to
FlatSlider— dragging the track/knob now moves the value continuously with the cursor, matching how a native<input type="range">behaves.Stack: #2120 → ... → #2142 → #2186 (this).
Why
Reported directly after using the redesigned inspector live — only
onPointerDownwas ever wired on the slider's track, so a click set the value once but dragging did nothing further; the value stayed pinned to wherever the mouse first went down.How
onPointerMove/onPointerUpalongside the existingonPointerDown, using the standard Pointer Capture API:setPointerCaptureon pointerdown so the element keeps receiving move events even if the cursor leaves its bounds mid-drag,onPointerMoverecomputes and commits the value from the currentclientXwhile capture is held, andonPointerUpreleases capture.SliderControluses a native<input type="range">, which gets drag for free from the browser;FlatSlideris a custom-drawn track/knob div with no such built-in behavior, so this needed explicit wiring.Test plan
setPointerCapturefor untrusted/script-dispatched pointer events, which is worth knowing if this needs re-testing later): drove an actual mouse-down + mouse-move sequence via CDP-level input and confirmed the slider's value tracked the drag's live position, not just the initial click.oxlint/oxfmt/fallowclean.